-
Notifications
You must be signed in to change notification settings - Fork 710
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pallet-utility: if_else
#6321
base: master
Are you sure you want to change the base?
pallet-utility: if_else
#6321
Conversation
User @rainbow-promise, please sign the CLA here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you understand the requirements clearly because you have implemented batch, not if-else.
the fallback is only executed if and only if the main call failed. if main call successes, do not call fallback
Need to take a closer look at the extrinsic executions, even with the revisions it Is not quit there yet. |
looks fine now. need proper weights |
I am guessing the weight signature below the pallet index will have some form of condition as well? |
use the worst case (i.e. main + fallback). extra weight will be refunded as the actual weight is returned in the call |
this also applies to dispatch class? |
I would say dispatch class is the lowest class of the 2. if both are operational then it is operational. if one is normal then it is normal. etc. |
how can one determine the class of a |
take a look at batch, they do similar operation. You can use |
Also the |
let caller = whitelisted_caller(); | ||
|
||
#[extrinsic_call] | ||
_(RawOrigin::Signed(caller), main_call, fallback_call); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The err branch should be slightly more costly, but difference should be negligible so it is ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
Thanks for the review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use a root call, it will be failing, like set_heap_page
.
This would benchmark for the failing case, which is more costly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...
#[benchmark]
fn if_else() {
let main_call = Box::new(frame_system::Call::set_heap_pages { pages: 0u64 }.into());
let fallback_call = Box::new(frame_system::Call::remark { remark: vec![1] }.into());
let caller = whitelisted_caller();
#[extrinsic_call]
_(RawOrigin::Signed(caller), main_call, fallback_call);
assert_last_event::<T>(
Event::IfElseFallbackCalled {
main_error: sp_runtime::DispatchError::Module(sp_runtime::ModuleError {
index: 0,
error: [5, 0, 0, 0],
message: None,
}),
}
.into(),
);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gui1117 the event passes for cargo test -p pallet-utility --features runtime-benchmarks
but fails when generating weights for runtimes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good now, thanks!
I just put some comments.
Review required! Latest push from author must always be reviewed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good now, thanks 🙏
Just some small comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
/tip medium |
@shawntabrizi A referendum for a medium (80 DOT) tip was successfully submitted for @rainbow-promise (1HbdqutFR8M535LpbLFT41w3j7v9ptEYGEJKmc6PKpqthZ8 on polkadot). |
The referendum has appeared on Polkassembly. |
Utility Call Fallback
This introduces a new extrinsic:
if_else
Which first attempts to dispatch the
main
call(s). If themain
call(s) fail, thefallback
call(s) is dispatched instead. Both calls are executed with the same origin.In the event of a fallback failure the whole call fails with the weights returned.
Use Case
Some use cases might involve submitting a
batch
type call in either main, fallback or both.Resolves #6000
Polkadot Address: 1HbdqutFR8M535LpbLFT41w3j7v9ptEYGEJKmc6PKpqthZ8